Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All the filesystem-related W* macros accept a filesystem context pointer as first parameter #556

Conversation

falemagn
Copy link
Contributor

For better portability.

@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

…ter as first parameter, to make portability better.
@falemagn falemagn force-pushed the pull-reqs/f4021bb_All_the_filesystem-related_W_macros_accept_a_filesystem_context_pointer_as_first_parameter branch from 1a073da to 092c04f Compare July 21, 2023 09:53
@JacobBarthelmeh JacobBarthelmeh self-assigned this Jul 21, 2023
@JacobBarthelmeh
Copy link
Contributor

ok to test

@JacobBarthelmeh
Copy link
Contributor

Have been reviewing this, and looked it over a couple times so far. My only concern is backwards compatibility if in some case the applications had overridden the W* file types with their own, then they would need to update their application when updating the wolfSSH version used.

@falemagn
Copy link
Contributor Author

Have been reviewing this, and looked it over a couple times so far. My only concern is backwards compatibility if in some case the applications had overridden the W* file types with their own, then they would need to update their application when updating the wolfSSH version used.

Our own application needs a filesystem context in all cases, how do you suggest to move forward? This topic is also covered by issue #558.

Copy link
Contributor

@ejohnstown ejohnstown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chance you can pass the ssh->fs ctx into all the filesystem functions, and add an accessor to set the fs ctx?

@falemagn
Copy link
Contributor Author

@ejohnstown I am not sure I understand. Which filesystem functions are we talking about? The accessor, as far as I can see and understand, is already there: wolfSSH_SetFilesystemHandle.

@ejohnstown
Copy link
Contributor

ejohnstown commented Aug 29, 2023

@ejohnstown I am not sure I understand. Which filesystem functions are we talking about?

I meant the function-style macros you added. For the fs parameter you passed in NULL, rather than the intended ssh->fs.

The accessor, as far as I can see and understand, is already there: wolfSSH_SetFilesystemHandle.

Whoops. I see the function wolfSSH_SetFilesystemHandle(). That's my mistake.

@falemagn
Copy link
Contributor Author

falemagn commented Aug 29, 2023

@ejohnstown I am not sure I understand. Which filesystem functions are we talking about?

I meant the function-style macros you added. For the fs parameter you passed in NULL, rather than the intended ssh->fs.

I passed NULL where NULL was already being passed to the other macros that already accepted the same parameter: those places make no use of the filesystem context, and I didn't change that.

As explained in issue #558, I believe the whole problem should be tackled differently, avoiding macros, but that issue hasn't got attention yet.

This patch simply goes in the same direction that has already been set.

@ejohnstown
Copy link
Contributor

This should be rebased with the current master branch. And it breaks the wolfSSHd build. The changes aren't that bad.

…ed_W_macros_accept_a_filesystem_context_pointer_as_first_parameter
@ejohnstown
Copy link
Contributor

I am going to merge this despite the tests failed. I'm going to PR a fix right after merging this.

@ejohnstown ejohnstown merged commit 0126528 into wolfSSL:master Sep 1, 2023
@ejohnstown ejohnstown mentioned this pull request Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants